[BUILD] Improve the handling of OPENTELEMETRY_HAVE_WORKING_REGEX. #2430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It used to be some places were checking for defined(OPENTELEMETRY_HAVE_WORKING_REGEX), but it is always being set (to either 0 or 1) in api/include/opentelemetry/common/macros.h. Thus move to just checking its value everywhere. Additionally, even if we did have the feature off - compilation would fail, since not all the functions were ifdef'd. My initial motivation for this was to reduce compile times, since specificing the definition with an std::regex adds a significant penalty to each translation unit (~700ms on my 5-year old laptop). In a separate patch, I think this should be moved to a .cc file - happy to receive advice on how (since api/ seems not to have .cc's).
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
==========================================
+ Coverage 87.04% 87.06% +0.02%
==========================================
Files 199 199
Lines 6079 6079
==========================================
+ Hits 5291 5292 +1
+ Misses 788 787 -1
|
lalitb
approved these changes
Dec 5, 2023
ThomsonTan
approved these changes
Dec 5, 2023
marcalff
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It used to be some places were checking for
defined(OPENTELEMETRY_HAVE_WORKING_REGEX), but it is always being set (to either 0 or 1) in api/include/opentelemetry/common/macros.h.
Thus move to just checking its value everywhere.
Additionally, even if we did have the feature off - compilation would fail, since not all the functions were ifdef'd. I assume this fails since there are not tests that have OPENTELEMETRY_HAVE_WORKING_REGEX as off.
My initial motivation for this was to reduce compile times, since specificing the definition with an std::regex adds a significant penalty to each translation unit (~700ms on my 5-year old laptop). In a separate patch, I think this should be moved to a .cc file - happy to receive advice on how (since api/ seems not to have .cc's).